add support for lambda expressions#8
Conversation
| * @param body the body of the function. | ||
| */ | ||
| public Builder addLambda(Iterable<ParameterSpec> parameters, boolean emitTypes, CodeBlock body) { | ||
| int paramsLen = 0; |
There was a problem hiding this comment.
I'd move these checks into the processing of the parameter stream below. This way we don't go over the parameters list twice. It's small but no reason to double iterate the list.
There was a problem hiding this comment.
I pushed latest. If you agree with the approach i can flag it as resolved, thanks.
| * be emitted on the result. | ||
| * @param body the body of the function. | ||
| */ | ||
| public Builder addLambda(Iterable<ParameterSpec> parameters, boolean emitTypes, CodeBlock body) { |
There was a problem hiding this comment.
wdyt of making emitTypes an enum instead. Something like public enum LambdaMode .... This is a better U/X for users and allows us to add more modes should we ever need to.
There was a problem hiding this comment.
Thats sounds like a good improvement and thank your for your comment! Are you thinking something like enum {showTypes;} for type visibility and we can add more values if we need later?
There was a problem hiding this comment.
I pushed latest, the logic stays pretty much the same, but now we can add various format rules if we need to in the future. When it is needed, we just add a mode to LambdaModes enum and add the case to the parent addLambda method (in CodeBlock). I added 2 new overloads for when the user defines a lambda with no inputs (so he can define a mode, or not and the default one is used) in case a new mode is added in the future that can be applied to said lambdas. Right now, of course, the overloads dont provide discrete value (because when a lambda has no inputs it is moot to specify its input type visibility), but they will seem useful when new modes get added in the LambdaModes enum (that can possible be applied to lambdas with no inputs)
| } | ||
|
|
||
| // the body of the lambda (right side) | ||
| String bodySide = body.toString().replaceAll("\n", ""); |
There was a problem hiding this comment.
This seems wrong. Why are we doing this? We should not reformat code that has already been specified by users.
There was a problem hiding this comment.
wdyt about having a new line after the { so for body inputs like:
int x = 6;
int y = 7;
return x + y;
we get outputs like:
input -> {
int x = 6;
int y = 7;
return x + y;
}
(or with proper indents if you think thats better).
By leaving the formatted user input as is, without removing new lines, we might fall into outputs like:
input -> {int x =6;
int y = 7;
return x + y;
}
There was a problem hiding this comment.
The braces placement should be customizable. No one will agree on where to put it. This is why I've been suggesting we have a LambdaSpec class. I don't see any way around it. If we had a LambdaSpec builder class we wouldn't need so many method overloads. This would also pay benefits for the switch PR. the body after the -> arm of a switch is just like a lambda and a builder would help there.
I'd like to see all these method overloads moved into a builder class, possibly with a bas class that can re-used for the switch PR. The placement of the lambda body braces should be configurable just as it is in ClodeBlock
|
|
||
| // in case of multiple statements, braces are mandatory | ||
| if (bodySide.contains(";")) { | ||
| bodySide = "{" + bodySide + "}"; |
There was a problem hiding this comment.
This is forcing a coding style. Users need to be able to specify newlines, indents, etc. Many people will not like the output of this.
There was a problem hiding this comment.
The emitted braces are for lambdas that contain a full method body such as x -> {int y=5; return x + 5;} in contrast with the more common lambdas x -> x + 5 that dont have mutlitple statements. I dont exaclty understand what you mean about forcing a code style, since that braces in the first example are mandatory to producing valid java syntax. Are you saying that the user should be responsible for placing the braces in its code blocks if he wishes to have mutliple statements, so just remove lines 460-462? About the new lines and indents, since the body is a CodeBlock, the user can definitely place indents and new lines in the body if he wishes to, when creating the CodeBlock. Some further insights are very welcome!
There was a problem hiding this comment.
Indenting, newlines, etc. must all be configurable by users.
| } | ||
|
|
||
| // the full lambda structure | ||
| add(inputSide + " -> " + bodySide); |
There was a problem hiding this comment.
Assuming spaces around -> is wrong. Like all things, we must allow users to specify how code is formatted. FYI - this is why I suggested a separate LambdaBlock class. There are simply too many options to force into method arguments.
There was a problem hiding this comment.
My idea was based on the rest of the codebase that seems to follow that kind of pattern. We have methods such as beginControlFlow() for example that does indeed emitt spaces before { to produce the output and doesnt allow user configuration. I take that this is coded that way because spaces before { on if statements and for loops, are a standard for java code.
3adf8bc to
6518576
Compare
feature used to structure anonymous functions with CodeBlock and within MethodSpec
|
@HliasMpGH is there anyone else you know who can help review this? |
fd201f7 to
4478030
Compare
|
The implementation of the LambdaSpec class can be seen in #16. Closing |
FIRST PART OF ISSUE #5
This feature allows the use of lambda expressions in CodeBlocks and within MethodSpecs.
A usage of the feature can be:
to produce:
The visibility of the input types in the result is a conditional that can be configured as follows:
that produces:
or:
to get:
It is not required to specify if the body of the lambda is an expression or not when using the feature. The output will reflect, depending on the inputs, the wanted and most importantly, valid, result.
if multiple statements are given as a body to the lambda, in the ouput it will be included in braces.